Skip to content

fix(netty): preserve QUERY across redirects - #2317

Merged
hyperxpro merged 1 commit into
AsyncHttpClient:mainfrom
mkurz:fix/query-redirects
Sep 5, 2026
Merged

fix(netty): preserve QUERY across redirects#2317
hyperxpro merged 1 commit into
AsyncHttpClient:mainfrom
mkurz:fix/query-redirects

Conversation

@mkurz

@mkurz mkurz commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Preserve the QUERY method and request content when following 301, 302, 307, and 308 redirects, as required by RFC 10008.
  • Keep the existing 303 behavior, which follows the redirect with GET and drops the request content.
  • Add a HttpConstants.Methods.QUERY constant and name the redirect-policy decisions explicitly.
  • Cover every QUERY redirect status, strict and non-strict 302 handling, existing POST/PUT/PATCH/DELETE behavior, repeatable and non-repeatable generated bodies, and cross-origin redirects.

Problem

Redirect30xInterceptor treated every method other than GET, HEAD, and OPTIONS like POST when handling 301 and non-strict 302 responses. As a result, a QUERY request was changed to GET and its query content and Content-Type were dropped.

RFC 10008 section 2.5 explicitly says that the POST-to-GET exceptions for 301 and 302 do not apply to QUERY. A QUERY request must instead be repeated with its content for 301, 302, 307, and 308. Only a 303 response calls for a GET request to the redirect target.

Change

Recognize QUERY in the redirect policy so 301 and 302 retain the original method and body. The existing 307, 308, 303, HEAD, OPTIONS, and POST behavior is unchanged. The implementation uses named decisions rather than embedding the QUERY exception in one compound expression.

Cross-origin QUERY redirects retain their method, content, and Content-Type, as required to repeat the query. This means a QUERY body now crosses origins on 301 and 302 where the old, incorrect GET rewrite dropped it. AHC already uses that body-replay trust model for 307 and 308. Existing redirect security still strips Authorization, Realm credentials, and user-supplied Cookie headers before sending the request to the new origin. A separate, representation-independent policy that refuses cross-origin keep-body redirects could be considered, but this conformance fix does not currently add one.

The broader pre-existing behavior that converts PUT, PATCH, DELETE, and other methods to GET after 301 and non-strict 302 responses is deliberately out of scope. It changes established behavior for existing users and is handled in a separate follow-up branch, fix/non-post-redirects.

That follow-up handles QUERY as an ordinary non-POST method and therefore supersedes this pull request's QUERY-specific interceptor condition if both land. Keeping this narrow pull request separate still allows the standardized QUERY behavior to land even if the broader compatibility change is rejected; the public method constant and QUERY-specific regression coverage remain useful either way.

This adds the public HttpConstants.Methods.QUERY string constant. It is an additive, user-facing API for constructing QUERY requests; there is no incompatible API change.

AI disclosure

OpenAI Codex on behalf of Matthias Kurz. The commits include Co-Authored-By: OpenAI Codex <codex@openai.com> per AGENTS.md.

Test plan

  • Reproduced the failure on JDK 11 with ./mvnw -pl client -Dtest=RedirectBodyTest#query301KeepsMethodAndBody test.
  • ./mvnw -pl client -Dtest=RedirectBodyTest test on JDK 11.
  • ./mvnw -pl client -Dtest=RedirectBodyTest,RedirectCredentialSecurityTest test on JDK 11: 57 tests passed.
  • ./mvnw clean verify on JDK 11: BUILD SUCCESS (full reactor, including tests, Javadocs, artifact signing, coverage, and Revapi).

Generated with OpenAI Codex.

@mkurz

mkurz commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

We use ahc in https://github.com/playframework/play-ws and I am in the process of upgrading to v3 - and found some thing worth adressing.

Comment thread client/src/main/java/org/asynchttpclient/util/HttpConstants.java Outdated
Comment thread client/src/test/java/org/asynchttpclient/RedirectBodyTest.java
Comment thread client/src/test/java/org/asynchttpclient/RedirectBodyTest.java
@mkurz

This comment was marked as outdated.

@mkurz
mkurz force-pushed the fix/query-redirects branch from b8c4db4 to 4596aa0 Compare August 31, 2026 22:44
@mkurz
mkurz requested a review from hyperxpro August 31, 2026 22:51
@mkurz

This comment was marked as outdated.

@mkurz

mkurz commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@hyperxpro I have addressed all concrete review feedback in five follow-up commits and rerun ./mvnw clean verify successfully. The implementation is ready from my side.

Two policy decisions remain:

  1. For cross-origin body replay, my preference is to retain AHC’s existing keep-body policy in this conformance fix and discuss a representation-independent option that refuses all cross-origin keep-body redirects separately.
  2. For scope, I have a local branch that limits the legacy rewrite to POST and therefore subsumes the QUERY-specific interceptor condition here. I lean toward replacing this PR with that broader change, but keeping this PR allows QUERY conformance to land independently if the PUT/PATCH/DELETE compatibility change needs more discussion.

Please let me know which scope you prefer and whether retaining the existing cross-origin policy is acceptable here.

@hyperxpro hyperxpro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last round

@mkurz
mkurz force-pushed the fix/query-redirects branch from 69393b5 to 0a6b003 Compare September 4, 2026 09:05
@mkurz

mkurz commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Since I can no longer reply inline after the force-push:

Fine by me, let's keep the existing trust model here. Two things I got wrong above: 301 and 308 are the heuristically cacheable ones, not 301 and 302, and we already replay on 308, so caching isn't the difference. And forget the sameBase idea, you were right, it would keep the method and drop the body. My worry is just that 301 and 302 are what a redirect helper emits by default, so that's what an open redirect gives you, while 307 and 308 have to be asked for. But that's about the interceptor in general, not this PR. I'll open a separate issue.

Thanks for clarifying, and for taking the general follow-up issue. I kept AHC's existing cross-origin keep-body trust model unchanged in this PR. The branch is now rebased onto latest main and squashed to 94bacf1; ./mvnw clean verify passes on JDK 11.

I'd keep this one. Your follow up also makes PUT, PATCH and DELETE keep their bodies on 301 and 302, and that's a much bigger change to behaviour that's been there since 2015. QUERY is new so nobody depends on it yet, it's the safer one to go first. Also for the squash message, the PR body mentions a copyBody helper from #2316 but there's no such method, it landed as request.toBuilder().

Agreed. I kept this PR narrowly scoped to QUERY; the existing PUT, PATCH, and DELETE behavior remains unchanged and is pinned by regression tests. The broader POST-only rewrite remains a separate follow-up.

Also corrected: #2316 landed the replay preservation through request.toBuilder(), not a copyBody helper. Neither the current PR description nor the final squash commit message refers to copyBody.

The branch is now rebased onto latest main and squashed to 94bacf1; ./mvnw clean verify passes on JDK 11.

RFC 10008 requires QUERY requests to retain their method and content
across 301, 302, 307, and 308 redirects. AHC treated QUERY like POST
on 301 and non-strict 302, changing it to GET and dropping its content.

Preserve QUERY while keeping established behavior for other methods.
Add the standardized method constant and regression coverage for every
redirect status, strict 302, repeatable and non-repeatable bodies, and
cross-origin credential stripping.

Cross-origin QUERY redirects retain AHC's existing keep-body trust
model. A broader change limiting legacy redirect rewrites to POST
remains a separate compatibility decision.

OpenAI Codex on behalf of Matthias Kurz.

Co-Authored-By: OpenAI Codex <codex@openai.com>
@mkurz
mkurz force-pushed the fix/query-redirects branch from 0a6b003 to 94bacf1 Compare September 4, 2026 09:24
@mkurz
mkurz requested a review from hyperxpro September 4, 2026 09:42
@hyperxpro
hyperxpro merged commit 1866c0e into AsyncHttpClient:main Sep 5, 2026
13 checks passed
@hyperxpro

Copy link
Copy Markdown
Member

Thanks a lot!

@mkurz
mkurz deleted the fix/query-redirects branch September 5, 2026 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants